Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a project setting to control wireframe thickness #74387

Closed
wants to merge 1 commit into from
Closed

Add a project setting to control wireframe thickness #74387

wants to merge 1 commit into from

Conversation

myaaaaaaaaa
Copy link
Contributor

@myaaaaaaaaa myaaaaaaaaa commented Mar 4, 2023

Makes wireframes easier to see on high resolution displays. A previous version of this setting force-enabled wireframes, but this has been retired in favor of finding ways to reuse Viewport::DEBUG_DRAW_WIREFRAME.

@myaaaaaaaaa myaaaaaaaaa requested review from a team as code owners March 4, 2023 20:17
@myaaaaaaaaa myaaaaaaaaa changed the title Add a debug setting to force wireframe rendering Add a debug/ project setting to force wireframe rendering Mar 4, 2023
@myaaaaaaaaa myaaaaaaaaa changed the title Add a debug/ project setting to force wireframe rendering Add a debug/rendering/force_wireframe project setting Mar 4, 2023
@Chaosus Chaosus added this to the 4.1 milestone Mar 6, 2023
main/main.cpp Outdated Show resolved Hide resolved
@clayjohn clayjohn modified the milestones: 4.1, 4.x May 23, 2023
main/main.cpp Outdated Show resolved Hide resolved
@Calinou
Copy link
Member

Calinou commented Jun 5, 2023

The way this is implemented makes lines pretty jittery when the depth prepass is enabled:

image

This is not an issue when the depth prepass is disabled or when using the wireframe debug draw mode, as both make all geometry see-through.

We should have a polygon offset for wireframe lines (similar to glPolygonOffset() in OpenGL), as mentioned in godotengine/godot-proposals#1000.

@myaaaaaaaaa
Copy link
Contributor Author

The way this is implemented makes lines pretty jittery when the depth prepass is enabled.

This is not an issue when the depth prepass is disabled or when using the wireframe debug draw mode, as both make all geometry see-through.

We should have a polygon offset for wireframe lines (similar to glPolygonOffset() in OpenGL), as mentioned in godotengine/godot-proposals#1000.

One possible workaround for this would be to set debug/rendering/force_wireframe twice as wide to compensate for half of it clipping into the original geometry. On my machine, line widths other than 1 lack MSAA support, but TAA is able to compensate for this.


I'm having second thoughts about this approach. Viewport::DEBUG_DRAW_WIREFRAME already exists, so debug/rendering/force_wireframe just ends up introducing redundant functionality. It would be better to have some way to force enable any of the Viewport::DEBUG_DRAW_* options at runtime, which would also allow the hypothetical outline mode to be seamlessly supported if it's ever added.

I'll leave this PR open for now though, in case anyone is interested in using it as a stopgap solution.

@myaaaaaaaaa myaaaaaaaaa marked this pull request as draft June 6, 2023 01:14
@Calinou
Copy link
Member

Calinou commented Jun 6, 2023

It would be better to have some way to force enable any of the Viewport::DEBUG_DRAW_* options at runtime, which would also allow the hypothetical outline mode to be seamlessly supported if it's ever added.

I agree with that too – it might also be worth having an option to synchronize the debug draw mode of the first 3D editor viewport with the running project, similar to camera replication.

@myaaaaaaaaa myaaaaaaaaa changed the title Add a debug/rendering/force_wireframe project setting Add a project setting to control wireframe thickness Jun 7, 2023
@myaaaaaaaaa myaaaaaaaaa marked this pull request as ready for review June 7, 2023 18:23
@myaaaaaaaaa
Copy link
Contributor Author

Repurposing this as a rendering/driver/wireframe/width setting

@myaaaaaaaaa myaaaaaaaaa deleted the force-wireframe branch September 28, 2023 23:33
@Zireael07
Copy link
Contributor

Probably salvageable?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants